Implement `Display` for `Path` and `PathBuf` #49868

withoutboats · 2018-04-11T10:12:23Z

In RFC #474, the issue of how to handle Displaying a Path was left as an open question. The problem is that a Path may contain non-UTF-8 data on most platforms. In the implementation of the RFC, a display method was added, which returns an adapter that implements Display by replacing non-UTF8 data with a unicode replacement character.

Though I can't find a record of the discussion around this issue, I believe there were two primary reasons not to just implement this behavior as the Display impl of Path:

The adapter allocated in the non-UTF8 case, and Rust as a rule tries to avoid allocations that are not explicit in code.
The user may prefer an alternative solution than using the unicode replacement character for handling non-UTF8 data.

In my view, the choice to provide an adapter rather than implement Display has had a high cost in terms of user experience:

I almost never remember that I need an adapter, forcing me to go back and edit my code after compiling it and getting an error.
It makes my code more noisy to have the display adapter; this detail is rarely important.
It is extremely uncommon to actually do something other than call the display adapter when trying to display a path (I have never wanted anything else myself).
For new users, it is Yet Another Compiler Error that they have to figure out how to solve, contributing to the sense that Rust nags and obstructs rather than assists & guides.

Therefore, I think time has shown that this has been a detriment to user experience, rather than a helpful reminder. That leaves only the first reason not to implement this: implicit allocations. That problem was happily resolved in June 2017: #42613 provided an alternative implementation which efficiently avoids allocations.

Given that, I think it is time that we implement Display for both Path and PathBuf and deprecate the Path::display method.

r? @alexcrichton
cc @rust-lang/libs

@alexcrichton

In [RFC rust-lang#474][rfc474], the issue of how to handle Displaying a Path was left as an open question. The problem is that a Path may contain non-UTF-8 data on most platforms. In the implementation of the RFC, a `display` method was added, which returns an adapter that implements `Display` by replacing non-UTF8 data with a unicode replacement character. Though I can't find a record of the discussion around this issue, I believe there were two primary reasons not to just implement this behavior as the `Display` impl of `Path`: 1. The adapter allocated in the non-UTF8 case, and Rust as a rule tries to avoid allocations that are not explicit in code. 2. The user may prefer an alternative solution than using the unicode replacement character for handling non-UTF8 data. In my view, the choice to provide an adapter rather than implement Display has had a high cost in terms of user experience: * I almost never remember that I need an adapter, forcing me to go back and edit my code after compiling it and getting an error. * It makes my code more noisy to have the display adapter; this detail is rarely important. * It is extremely uncommon to actually do something other than call the display adapter when trying to display a path (I have never wanted anything else myself). * For new users, it is Yet Another Compiler Error that they have to figure out how to solve, contributing to the sense that Rust nags nags and obstructs rather than assists & guides. Therefore, I think time has shown that this has been a detriment to user experience, rather than a helpful reminder. That leaves only the first reason not to implement this: implicit allocations. That problem was happily resolved in June 2017: rust-lang#42613 provided an alternative implementation which efficiently avoids allocations. Given that, I think it is time that we implement `Display` for both `Path` and `PathBuf` and deprecate the `Path::display` method. r? @alexcrichton cc @rust-lang/libs [rfc474]: https://github.com/rust-lang/rfcs/blob/master/text/0474-path-reform.md)

withoutboats · 2018-04-11T10:15:05Z

Probably checkboxes are justified for this:

@rfcbot fcp merge

rfcbot · 2018-04-11T10:15:09Z

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

correctness (Implement Display for Path and PathBuf #49868 (comment))

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

TimNN · 2018-04-11T10:19:03Z

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:00:46] configure: rust.quiet-tests     := True
---
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/lib.rs:28:74
[00:05:04]    |
[00:05:04] 28 |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]    |                                                                          ^^^^^^^
[00:05:04]    |
[00:05:04]   ::: tools/tidy/src/cargo.rs:26:18
[00:05:04]    |
[00:05:04] 26 |     for entry in t!(path.read_dir(), path).map(|e| t!(e)) {
[00:05:04]    |                  ------------------------- in this macro invocation
[00:05:04]    |
[00:05:04]    = note: `-D deprecated` implied by `-D warnings`
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/lib.rs:28:74
[00:05:04]    |
[00:05:04] 28 |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]    |                                                                          ^^^^^^^
[00:05:04] ...
[00:05:04] 90 |     for entry in t!(fs::read_dir(path), path) {
[00:05:04]    |                  ---------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/lib.rs:28:74
[00:05:04]    |
[00:05:04] 28 |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]    |                                                                          ^^^^^^^
[00:05:04]    |
[00:05:04]   ::: tools/tidy/src/bins.rs:50:24
[00:05:04]    |
[00:05:04] 50 |         let metadata = t!(fs::symlink_metadata(&file), &file);
[00:05:04]    |                        -------------------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/bins.rs:65:73
[00:05:04]    |
[00:05:04] 65 |                 tidy_error!(bad, "binary checked into source: {}", file.display());
[00:05:04]    |                                                                         ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/lib.rs:28:74
[00:05:04]     |
[00:05:04] 28  |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]     |                                                                          ^^^^^^^
[00:05:04]     |
[00:05:04]    ::: tools/tidy/src/style.rs:123:12
[00:05:04]     |
[00:05:04] 123 |         t!(t!(File::open(file), file).read_to_string(&mut contents));
[00:05:04]     |            -------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/style.rs:126:53
[00:05:04]     |
[00:05:04] 126 |             tidy_error!(bad, "{}: empty file", file.display());
[00:05:04]     |                                                     ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/style.rs:136:52
[00:05:04]     |
[00:05:04] 136 |                 tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg);
[00:05:04]     |                                                    ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/style.rs:172:60
[00:05:04]     |
[00:05:04] 172 |             tidy_error!(bad, "{}: incorrect license", file.display());
[00:05:04]     |                                                            ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/style.rs:175:72
[00:05:04]     |
[00:05:04] 175 |             0 => tidy_error!(bad, "{}: missing trailing newline", file.display()),
[00:05:04]     |                                                                        ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/style.rs:177:79
[00:05:04]     |
[00:05:04] 177 |             n => tidy_error!(bad, "{}: too many trailing newlines ({})", file.display(), n),
[00:05:04]     |                                                                               ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/errors.rs:84:48
[00:05:04]    |
[00:05:04] 84 |             tidy_error!(bad, "{}:{}: {}", file.display(), line_num, line);
[00:05:04]    |                                                ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/lib.rs:28:74
[00:05:04]     |
[00:05:04] 28  |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]     |                                                                          ^^^^^^^
[00:05:04]     |
[00:05:04]    ::: tools/tidy/src/features.rs:110:12
[00:05:04]     |
[00:05:04] 110 |         t!(t!(File::open(&file), &file).read_to_string(&mut contents));
[00:05:04]     |            ---------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/features.rs:114:52
[00:05:04]     |
[00:05:04] 114 |                 tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg);
[00:05:04]     |                                                    ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/features.rs:293:50
[00:05:04]     |
[00:05:04] 293 |                                             file.display(),
[00:05:04]     |                                                  ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/features.rs:305:56
[00:05:04]     |
[00:05:04] 305 |                     tidy_error!(bad, "{}:{}: {}", file.display(), line, msg);
[00:05:04]     |                                                        ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/lib.rs:28:74
[00:05:04]     |
[00:05:04] 28  |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]     |                                                                          ^^^^^^^
[00:05:04]     |
[00:05:04]    ::: tools/tidy/src/features.rs:326:12
[00:05:04]     |
[00:05:04] 326 |         t!(t!(File::open(&file), &file).read_to_string(&mut contents));
[00:05:04]     |            ---------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]   --> tools/tidy/src/cargo.rs:96:55
[00:05:04]    |
[00:05:04] 96 |                               depends on it", libfile.display(), krate);
[00:05:04]    |                                                       ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/lib.rs:28:74
[00:05:04]     |
[00:05:04] 28  |         Err(e) => panic!("{} failed on {} with {}", stringify!($e), ($p).display(), e),
[00:05:04]     |                                                                          ^^^^^^^
[00:05:04]     |
[00:05:04]    ::: tools/tidy/src/pal.rs:112:8
[00:05:04]     |
[00:05:04] 112 |     t!(t!(File::open(file), file).read_to_string(contents));
[00:05:04]     |        -------------------------- in this macro invocation
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/pal.rs:130:67
[00:05:04]     |
[00:05:04] 130 |         tidy_error!(bad, "{}:{}: platform-specific cfg: {}", file.display(), line, cfg);
[00:05:04]     |                                                                   ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/deps.rs:229:42
[00:05:04]     |
[00:05:04] 229 |         panic!("{} does not exist", path.display());
[00:05:04]     |                                          ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/deps.rs:241:64
[00:05:04]     |
[00:05:04] 241 |             println!("invalid license {} in {}", license, path.display());
[00:05:04]     |                                                                ^^^^^^^
[00:05:04]
[00:05:04] error: use of deprecated item 'std::path::Path::display': Path and PathBuf implement Display directly
[00:05:04]    --> tools/tidy/src/deps.rs:248:43
[00:05:04]     |
[00:05:04] 248 |         println!("no license in {}", path.display());
[00:05:04]     |                                           ^^^^^^^
[00:05:04]
[00:05:04] error: aborting due to 22 previous errors
[00:05:04]
[00:05:04] error: Could not compile `tidy`.
[00:05:04]
[00:05:04] Caused by:
[00:05:04]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name tidy tools/tidy/src/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=2 -C metadata=ec0ed652cc88252d -C extra-filename=-ec0ed652cc88252d --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/release/deps --extern serde_derive=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/release/deps/libserde_derive-6fd95ab87191bb85.so --extern serde_json=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps/libserde_json-b3e872b56e3c8f47.rlib --extern serde=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps/libserde-92847b096fc60ebd.rlib` (exit code: 101)
[00:05:04] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/tidy/Cargo.toml" "--features" "" "--message-format" "json"
[00:05:04] expected success, got: exit code: 101
[00:05:04] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:04] Build completed unsuccessfully in 0:01:50
[00:05:04] make: *** [tidy] Error 1
[00:05:04] Makefile:79: recipe for target 'tidy' failed
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:03b47ecb:start=1523441936035098256,finish=1523441936041781453,duration=6683197
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:1c787166
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:1c787166:start=1523441936047411082,finish=1523441936053841006,duration=6429924
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:03634257
$ dmesg | grep -i kill
[   11.039019] init: failsafe main process (1092) killed by TERM signal

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

withoutboats · 2018-04-11T10:34:58Z

So what I've learned from the logs is that deprecating this is going to cause a lot of warnings. Possibly we should just provide the new API without deprecation?

BurntSushi · 2018-04-11T10:46:14Z

Strong 👍 from me on this. I work with paths a lot, and the number of times I've benefited from the fact that display() is implemented as an adapter rather on the type itself is exactly zero. :-)

I feel like deprecating the display adapter long term is a fine thing to do, but deprecating it now (or even too soon after stabilizing the Display impl), would make the transition much more annoying than it needs to be.

SimonSapin · 2018-04-11T10:57:09Z

In general, we shouldn’t emit deprecation warnings in Nightly before the recommended replacement is available at least in the Stable channel.

But I’m not convinced we should mako this specific change.

@rfcbot concern correctness

I believe that the reason not to implement Display is that, since std::fmt is Unicode-based, an implementation must be lossy: its behavior cannot be relied on to later go back to &Path or PathBuf. The concern was that this could cause bugs that would be hard to diagnose because they only occur with non-Unicode paths.

withoutboats · 2018-04-11T13:06:01Z

@SimonSapin

It seems overwhelmingly more common to want to display paths for user consumptions than to try to use the display impl to "round trip" them. I also suspect that these issues would not be hard to debug - they seem very likely to appear as some invalid path which contains the unicode replacement character (I'd agree with the contention that they likely would not be caught in testing unless you're doing fuzz testing with randomly generated valid paths.) And I'm not convinced that forcing users to go .display() is all that likely to get them to realize and respond to the correctness issue when they are roundtripping.

TimNN · 2018-04-11T13:09:17Z

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:00:49] configure: rust.quiet-tests     := True
---
[00:04:58] tidy error: /checkout/src/libstd/path.rs:1496: mismatches to previous in: ["since"]
[00:04:58] tidy error: /checkout/src/libstd/path.rs:1503: mismatches to previous in: ["since"]
[00:04:58] tidy error: /checkout/src/libstd/path.rs:2490: mismatches to previous in: ["since"]
[00:04:58] tidy error: /checkout/src/libstd/path.rs:2497: mismatches to previous in: ["since"]
[00:04:59] some tidy checks failed
[00:04:59]
[00:04:59]
[00:04:59] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:59] expected success, got: exit code: 1
[00:04:59]
[00:04:59]
[00:04:59] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:59] Build completed unsuccessfully in 0:01:55
[00:04:59] Makefile:79: recipe for target 'tidy' failed
[00:04:59] make: *** [tidy] Error 1
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:104ce914:start=1523452151645583261,finish=1523452151653736476,duration=8153215
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:0526cb85
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:0526cb85:start=1523452151661886551,finish=1523452151670246274,duration=8359723
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0b0abae4
$ dmesg | grep -i kill
[   10.409696] init: failsafe main process (1092) killed by TERM signal

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

BurntSushi · 2018-04-11T13:37:11Z

@SimonSapin An interesting case where this comes up easily is error messages. Namely, I'm pretty sure I almost always use path.display() when formatting an error message with a path, even though that is actually technically incorrect, and could show a file path to an end user that does not actually correspond to a path on their system. The Unicode replacement character would be a tipoff I guess when that happens.

My point here is that, even with the adapter method, I'm already doing the technically incorrect thing, largely because it seems border line impossible to do the right thing. Namely, how am I supposed to print an error message that might not be valid UTF-8? Our error messages are irrevocably tied to valid UTF-8 because our formatting infrastructure is tied to valid UTF-8. I would need to seriously go out of my way to get this right. Now, I don't think this is a huge deal in and of itself, but the display method isn't really doing much for me here.

On the other hand, there are cases where a program wants to print a file path for non-error reasons, e.g., an ls-like program. When I write those sorts of programs, I do care enough to make sure the file paths are roundtripped losslessly when possible. And that code is not at all obvious to write. Here's what I've written in the past:

    #[cfg(unix)]
    fn write_path<P: AsRef<Path>>(&mut self, path: P) -> Result<()> {
        use std::os::unix::ffi::OsStrExt;
        let path = path.as_ref().as_os_str().as_bytes();
        self.wtr.write_all(path)
    }

    #[cfg(not(unix))]
    fn write_path<P: AsRef<Path>>(&mut self, path: P) -> Result<()> {
        let path = path.as_ref().to_string_lossy();
        self.wtr.write_all(path.as_bytes())
    }

In other words, I not only need to abandon our nice formatting infrastructure, but I need to write platform dependent code to do it.

I'm with @withoutboats on this one. While I appreciate the display() method as a possible road bump to notifying users of potential correctness problems, those problems are incredibly thorny, and I don't think the adapter method carries its weight because I'm not convinced that it actually nudges folks in the right direction.

alexcrichton · 2018-04-11T14:15:48Z

I definitely sympathize with the problems mentioned here, I run into them all the time myself as well. I feel, though, that we've got a lot of messaging to undo if we start down this road (which I'm not sure we want to do). For example the documentation for std::fmt states:

fmt::Display implementations assert that the type can be faithfully represented as a UTF-8 string at all times. It is not expected that all types implement the Display trait.

which directly implies that Display for Path cannot exist. We can of course change this wording, but I feel like that also has much larger ramifications. Working with Path is often brought up as either the worst or the best part of Rust. It's not great because it's so unergonomic and there's so many unwrap or Display gotchas. It's also sometimes the best because it has all these gotchas which have been bugs in real-world programs.

Along those lines, though, I would have a question of why stop here? If our goal is to make Path more easily usable why stop at Display for Path? Why not make parent return Path? Why not add Display for OsStr? etc/etc.

Is there a strong rationale for stopping here and not adding further conveniences?

withoutboats · 2018-04-11T14:25:51Z

Is there a strong rationale for stopping here and not adding further conveniences?

To me this seems like a very strange way to frame the question! Every decision is a trade off & should be judged on its own merits. Why allow the display adapter at all - I believe most users use it thoughtlessly without question & don't actually consider whether its correct; why shouldn't we make them jump through more hoops to lossily display their Paths and potentially introduce errors? Why should Option, which parent returns, have an unwrap convenience when it can be the source of runtime panics? Etc etc, you can ask infinite questions in either direction on the line between Coq and JavaScript.

We can't make some sort of sweeping philosophical argument in a great "convenience vs correctness" debate. For each API decision we have to ask: is this choice improving users' lives & work? I think this is a case where our historical decision was a clear net negative for users.

BurntSushi · 2018-04-11T14:50:49Z

@alexcrichton Yeah I'm not sure if there is a super strong rationale separating these things. It's definitely a little blurry, so we'd probably need to employ reasoning like, "paths are frequently used and printed, and the specific display adapter we have for it isn't carrying its weight." It's not an especially generic argument that can be applied to other things I think.

The docs on fmt::Display are definitely interesting. Not sure what to make of that.

WiSaGaN · 2018-04-11T14:56:48Z

Does deprecating a stable interface not require a RFC?

Edit: If we need a RFC to deprecate a stable interface, I think introducing a replacement of the current interface without deprecating also needs a RFC:

If we do not follow up with a deprecation, we are introducing another way to do the same thing conflicting with the existing one.
If we follow up with a deprecation, we need a RFC in the first place, thus we might as well do it before hand to reduce the churn.

retep998 · 2018-04-11T15:43:27Z

The problem with implementing Display directly is now you can call to_string directly which carries no connotation that the conversion is lossy and it has the convenience of not failing, which will cause people to reach for to_string instead of the current situation where they have to use to_string_lossy which is clear about its lossiness or to_str which returns an Option so they are aware it may fail.

Strong opposition from me on this change.

alexcrichton · 2018-04-11T17:29:47Z

@withoutboats I agree in general yes but I'd personally find the decision to only add Display for Path (and PathBuf) to be somewhat incoherent. For example why would we add it to these types and not OsString? This means that you could print a path but you couldn't print the return value of file_name().

Put another way I feel like there's a lot of "papercuts" when working with Path which are actually intentional design decisions. While some papercuts come up more often than others I think that the fact that it's a papercut doesn't necessarily undermine the original design decision, which in this case is ensuring lossy conversions are always opt-in rather than "idiomatically silent".

I think there's perhaps an argument to be made, however, that the "eat your vegetables" approach we have today ends up benefitting nearly no one. Those who are aware of the issues don't necessarily need the constant ergonomic overhead and those who aren't aware of the issues are likely to do the (subtly) wrong thing anyway.

Even if this provides a strong motivation for this change though I think we have more issues to work through than just adding a Display implementation, for example re-rationalizing what Display means when implemented and the idiomatic relationship between Display and FromStr, for example

withoutboats · 2018-04-11T17:59:14Z

I think there's perhaps an argument to be made, however, that the "eat your vegetables" approach we have today ends up benefitting nearly no one. Those who are aware of the issues don't necessarily need the constant ergonomic overhead and those who aren't aware of the issues are likely to do the (subtly) wrong thing anyway.

Yes, this is my opinion.

Kimundi · 2018-04-12T08:56:28Z

I think I agree with @retep998 that the biggest footgun this opens up is enabling to_string().

Rust already has way to many conversion mechanisms that we try hard to keep somewhat sane, with beginners often just blindly trying stuff until it works. And most languages don't bother to keep strings and paths as separated as Rust does, so people often don't expect to look out for it.

But its also true that there already many ways to misuse path-string conversions in other ways, so I also understand seeing this as a needless hurdle in thew API.

withoutboats · 2018-04-12T13:29:39Z

I agree that the "biggest footgun" is to_string - in that it seems very unlikely someone is trying to roundtrip a path from printing it with "{}", so to_string is really the footgun. I think all the arguments about the frequency that someone does this vs how frequently they don't want to roundtrip it, and how ineffectual making them write .display().to_string() is to prevent them from doing it, all apply to to_string(). In other words, what we've been talking about this whole time is primarily to_string().

When this had a deprecation warning I immediately found lots of places checked into this repo that were doing exactly this display().to_string() instead of a "more correct" to_string_lossy().into_owned(). I did not investigate if any of them try to resuscitate a path from the string elsewhere in the program.

alexcrichton · 2018-04-12T16:18:08Z

I don't think I personally agree with to_string as being the only issue here, in Cargo we've had to specially treat serialization of paths to files or otherwise into a Write. These are 99% of the time intended for consumption by another tool, for example make or a future run of cargo. In these cases non-unicode paths on Windows translate to errors that end up getting propagated to users.

alexcrichton · 2018-04-12T16:19:05Z

Another way to think about this perhaps is that if you do want to "do the right thing" then it's nearly impossible to audit with Display for Path because that can be subtly used in so many different contexts. Otherwise grepping for to_string_lossy() or .display() is much easier to do.

alexcrichton · 2018-04-12T17:01:17Z

One way to approach this problem which I would personally be more comfortable with is to have Display panic or return an error on non-utf8 paths (probably panic). That to me upholds the critical contract here (it's lossless on Display) and matches at least my own personal experience of non-utf8 paths basically never existing and when they do the application is horribly broken in many other ways.

BurntSushi · 2018-04-12T17:05:37Z

@alexcrichton That's tempting, but the problem there is when formatting error messages. If you do format!("{}: file does not exist", path) and path contains invalid UTF-8, then you definitely don't want that panicking.

alexcrichton · 2018-04-12T18:04:54Z

@BurntSushi true yeah, but I feel like we have to give on something here. We've explicitly documented that we cannot do what this PR currently proposes, so something's gotta give.

I would personally prefer to stick to the "Display is lossless" guarantee moreso than not panicking, and our messaging would be that if you want to make sure you're compatible with non-utf8 paths then you should have a test case (as that's what you'd really need anyway). It's true that panics will come up from time to time, but it's sort of just another "here's a reminder you need to handle a bug here". It's not as obvious as option.unwrap() but it's arguably far rarer to occur than an unwrap

BurntSushi · 2018-04-12T18:12:57Z

@alexcrichton Yeah I guess what I'm trying to say is that if our choice is between "status quo" and "impl Display, but panic on invalid UTF-8," then I'd probably pick the status quo.

My opinion has weakened on this over time. I dunno what to do. The to_string problem is not something I had considered, and is a tough pill to swallow.

withoutboats · 2018-04-12T19:00:13Z

I don't think people use or understand display as lossless - I think they use and understand it as pretty printing intended for the end user. For example, the Display impl of the Context type in failure prints only the context and not the underlying error - two different Contexts with the same context but different errors will Display the same (and this wasn't my design, it was @aturon's). This is based on this much looser definition of Display which is how I think Display is used.

I'm unclear how the existing situation upholds that requirement either; why is the Display impl for the std::path::Display type "faithful" if the impls in this PR are not "faithful", when all they do is delegate to that impl? This definition seems no more rigorous than any other choice.

joshtriplett · 2018-04-13T18:31:00Z

This does seem like a case where people aren't clear on what the alternative is. If you really, really want to emit a non-UTF-8 path, presumably you'd have to treat the Path or PathBuf as bytes and Write those bytes. Doing so to a file, if the file format expects a raw path, can work. Doing so to stdout may or may not work, depending on if it goes to a UTF-8 terminal or to a pipe to some other program. But either way, do people know to do that, and know under what circumstances they should do that rather than just using Display (or .display())?

shepmaster · 2018-05-07T00:16:59Z

This has not had a comment in many weeks, so ping from triage.

retep998 · 2018-05-07T00:49:58Z

If you really, really want to emit a non-UTF-8 path, presumably you'd have to treat the Path or PathBuf as bytes and Write those bytes.

Which of course doesn't work work on Windows.

withoutboats · 2018-05-07T09:31:27Z

@aturon had an alternative proposal

pietroalbini · 2018-05-21T11:21:44Z

Ping from triage @rust-lang/libs! What's the consensus on this?

TimNN · 2018-06-12T17:46:59Z

Ping from triage @rust-lang/libs, what is the status of this?

SimonSapin · 2018-07-13T09:57:22Z

@TimNN I think the status is that there is no consensus so far.

@smmalis37 Disposition only means that one team member proposed FCP in that direction. #49868 (comment) lists a concern that blocks the start of FCP.

withoutboats · 2018-07-13T17:16:30Z

I think we don't have time to hash this out right now, will revisit in the future.

withoutboats added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 11, 2018

rust-highfive assigned alexcrichton Apr 11, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2018

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Apr 11, 2018

withoutboats added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2018

Remove deprecation.

a204487

Correct feature name.

980c5ec

withoutboats changed the title ~~Deprecate Path::display, implement Display for Path~~ Implement Display for Path and PathBuf Apr 11, 2018

jeekobu mentioned this pull request May 18, 2018

Ergonomics of String handling rust-cli/team#26

Open

Centril added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label May 24, 2018

stokhos added the I-nominated label Jun 29, 2018

withoutboats closed this Jul 13, 2018

rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 13, 2018

withoutboats removed I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 13, 2018

Mark-Simulacrum mentioned this pull request Aug 22, 2019

impl Display for Path #63822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `Display` for `Path` and `PathBuf` #49868

Implement `Display` for `Path` and `PathBuf` #49868

withoutboats commented Apr 11, 2018 •

edited

Loading

withoutboats commented Apr 11, 2018

rfcbot commented Apr 11, 2018 •

edited

Loading

TimNN commented Apr 11, 2018

withoutboats commented Apr 11, 2018

BurntSushi commented Apr 11, 2018

SimonSapin commented Apr 11, 2018

withoutboats commented Apr 11, 2018 •

edited

Loading

TimNN commented Apr 11, 2018

BurntSushi commented Apr 11, 2018

alexcrichton commented Apr 11, 2018

withoutboats commented Apr 11, 2018 •

edited

Loading

BurntSushi commented Apr 11, 2018

WiSaGaN commented Apr 11, 2018 •

edited

Loading

retep998 commented Apr 11, 2018 •

edited

Loading

alexcrichton commented Apr 11, 2018

withoutboats commented Apr 11, 2018

Kimundi commented Apr 12, 2018

withoutboats commented Apr 12, 2018 •

edited

Loading

alexcrichton commented Apr 12, 2018

alexcrichton commented Apr 12, 2018

alexcrichton commented Apr 12, 2018

BurntSushi commented Apr 12, 2018

alexcrichton commented Apr 12, 2018

BurntSushi commented Apr 12, 2018 •

edited

Loading

withoutboats commented Apr 12, 2018 •

edited

Loading

joshtriplett commented Apr 13, 2018

shepmaster commented May 7, 2018

retep998 commented May 7, 2018

withoutboats commented May 7, 2018

pietroalbini commented May 21, 2018

TimNN commented Jun 12, 2018

SimonSapin commented Jul 13, 2018

withoutboats commented Jul 13, 2018

Implement Display for Path and PathBuf #49868

Implement Display for Path and PathBuf #49868

Conversation

withoutboats commented Apr 11, 2018 • edited Loading

withoutboats commented Apr 11, 2018

rfcbot commented Apr 11, 2018 • edited Loading

TimNN commented Apr 11, 2018

withoutboats commented Apr 11, 2018

BurntSushi commented Apr 11, 2018

SimonSapin commented Apr 11, 2018

withoutboats commented Apr 11, 2018 • edited Loading

TimNN commented Apr 11, 2018

BurntSushi commented Apr 11, 2018

alexcrichton commented Apr 11, 2018

withoutboats commented Apr 11, 2018 • edited Loading

BurntSushi commented Apr 11, 2018

WiSaGaN commented Apr 11, 2018 • edited Loading

retep998 commented Apr 11, 2018 • edited Loading

alexcrichton commented Apr 11, 2018

withoutboats commented Apr 11, 2018

Kimundi commented Apr 12, 2018

withoutboats commented Apr 12, 2018 • edited Loading

alexcrichton commented Apr 12, 2018

alexcrichton commented Apr 12, 2018

alexcrichton commented Apr 12, 2018

BurntSushi commented Apr 12, 2018

alexcrichton commented Apr 12, 2018

BurntSushi commented Apr 12, 2018 • edited Loading

withoutboats commented Apr 12, 2018 • edited Loading

joshtriplett commented Apr 13, 2018

shepmaster commented May 7, 2018

retep998 commented May 7, 2018

withoutboats commented May 7, 2018

pietroalbini commented May 21, 2018

TimNN commented Jun 12, 2018

SimonSapin commented Jul 13, 2018

withoutboats commented Jul 13, 2018

Implement `Display` for `Path` and `PathBuf` #49868

Implement `Display` for `Path` and `PathBuf` #49868

withoutboats commented Apr 11, 2018 •

edited

Loading

rfcbot commented Apr 11, 2018 •

edited

Loading

withoutboats commented Apr 11, 2018 •

edited

Loading

withoutboats commented Apr 11, 2018 •

edited

Loading

WiSaGaN commented Apr 11, 2018 •

edited

Loading

retep998 commented Apr 11, 2018 •

edited

Loading

withoutboats commented Apr 12, 2018 •

edited

Loading

BurntSushi commented Apr 12, 2018 •

edited

Loading

withoutboats commented Apr 12, 2018 •

edited

Loading